Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support F_GETFL and F_SETFL for fcntl #4212

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

tiif
Copy link
Contributor

@tiif tiif commented Feb 26, 2025

This PR supports F_SETFL and F_GETFL flags for fcntl. In this implementation, F_SETFL can only set O_NONBLOCK flag.

If setfl is called while a fd is blocking, that fd will not be affected by the new flag value and will continue blocking like normal until a read/write wakes it up (it acts like it never knew the setfl has happened). But any operation after setfl will see the new flag value.

The interaction between these fcntl operations and blocking fd is summarised here.

Fixes #4119

Copy link
Contributor Author

@tiif tiif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rustbot ready

let [flag] = check_min_vararg_count(cmd_name, varargs)?;
let flag = this.read_scalar(flag)?.to_i32()?;

// FIXME: File access mode and file creation flags should be ignored.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the man page:

       F_SETFL (int)
              Set the file status flags to the value specified by arg.
              File access mode (O_RDONLY, O_WRONLY, O_RDWR) and file
              creation flags (i.e., O_CREAT, O_EXCL, O_NOCTTY, O_TRUNC)
              in arg are ignored.

There are quite a few file access mode and file creation flags, would prefer to open an issue for this and do it later.

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete labels Feb 26, 2025
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the first round of comments. Sorry for the long wait!

@rustbot author


// We only support F_GETFL for socketpair and pipe.
let anonsocket_fd = fd.downcast::<AnonSocket>().ok_or_else(|| {
err_unsup_format!("fcntl: only socketpair / pipe are supported for F_SETFL")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
err_unsup_format!("fcntl: only socketpair / pipe are supported for F_SETFL")
err_unsup_format!("fcntl: only socketpair / pipe are supported for F_GETFL")

})?;

if anonsocket_fd.is_nonblock() {
// socketpair's SOCK_NONBLOCK and pipe's O_NONBLOCK have the same value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fcntl docs seem to anyway indicate that we always return the O_* flags here. So this fact should not matter.

If it actually did matter, we should have an assert_eq! here.

err_unsup_format!("fcntl: only socketpair / pipe are supported for F_GETFL")
})?;

let cmd_name = "fcntl(fd, F_SETFL, ...)";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please inline this let.

Comment on lines +215 to +219
if flag == this.eval_libc_i32("O_NONBLOCK") {
anonsocket_fd.set_nonblock();
} else {
throw_unsup_format!("fcntl: only O_NONBLOCK are supported for F_GETFL")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also support un-setting the O_NONBLOCK flag, right? I assume that's what happens on flag == 0.

@@ -40,7 +40,7 @@ struct AnonSocket {
/// A list of thread ids blocked because the buffer was full.
/// Once another thread reads some bytes, these threads will be unblocked.
blocked_write_tid: RefCell<Vec<ThreadId>>,
is_nonblock: bool,
is_nonblock: Cell<bool>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
is_nonblock: Cell<bool>,
/// Whether this is a non-blocking socket or not.
is_nonblock: Cell<bool>,

Comment on lines +480 to +481
// 2. Thread 1 set O_NONBLOCK flag on fds[0], then check the value of F_GETFL.
// 3. Thread 2 writes to fds[1] to unblock the `read` in main thread.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this done by 2 different threads?

@rustbot rustbot removed the S-waiting-on-review Status: Waiting for a review to complete label Apr 9, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 9, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: Waiting for the PR author to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support F_SETFL and F_GETFL
3 participants